Security/NonceVerification: handle function names case correctly #2572
Conversation
The sniff was incorrectly treating function names as case-sensitive when checking for nonce verification functions. This led to false positives when developers used valid nonce verification functions with mixed or uppercase letters. The fix ensures function names are properly converted to lowercase before comparing against the allowed nonce verification functions list, respecting PHP's case-insensitive function name behavior.
There was a problem hiding this comment.
Good catch.
... respecting PHP's case-insensitive function name behavior.
PHP isn't completely case-insensitive for function names. Only for the ASCII characters and as this sniff allows for end-users to provide a list of customNonceVerificationFunctions via a public property, we cannot rely on all nonce-verification functions we need to check against being completely ASCII-based.
In other words, if someone would add a custom nonce verification function called déjà_vu(), this function would now never be recognized as a valid nonce verification function.
So, we can only use strtolower() to compare the names case-insensitively if we have a known list of functions we compare against and we are 100% sure all those function names are ASCII-based names.
When the list of functions to compare against is not known in advance and/or doesn't consist of all ASCII-based functions we need to loop through all "allowed functions" using the PHPCSUtils NamingConventions::isEqual() method - which does a comparison between two names in a way which is similar to how PHP does it.
I might be missing something, but I believe the sniff would handle Here is how I'm checking this: // phpcs:set WordPress.Security.NonceVerification customNonceVerificationFunctions[] déjà_vu
function non_ascii_characters() {
déjà_vu( 'something' ); // Ok.
update_post_meta( (int) $_POST['id'], 'a_key', $_POST['a_value'] );
}
function non_ascii_characters() {
DéJà_VU( 'something' ); // Ok.
update_post_meta( (int) $_POST['id'], 'a_key', $_POST['a_value'] );
}
function non_ascii_characters() {
dÉjÀ_vu( 'something' ); // Error, but that is correct as déjà_vu and dÉjÀ_vu are different function names.
update_post_meta( (int) $_POST['id'], 'a_key', $_POST['a_value'] );
}That being said, one thing that I realize only after seeing your comment is that if a function, even one with just ASCII characters, is added to // phpcs:set WordPress.Security.NonceVerification customNonceVerificationFunctions[] UPPERCASE_NAME
function non_ascii_characters() {
UPPERCASE_NAME( 'something' ); // Should be ok, but the sniff generates an error.
update_post_meta( (int) $_POST['id'], 'a_key', $_POST['a_value'] );
}Maybe using |
There was a problem hiding this comment.
@rodrigoprimo and me discussed this PR. Valid point about the call to strtolower(), however, that also means we need to ensure that the $customNonceVerificationFunctions are all lowercased before doing a comparison.
And we then also need to safeguard that via tests.
@rodrigoprimo Could you update the PR to ensure this is handled correctly please ?
…ly when looking for nonce verification functions
|
Thanks for your reply, @jrfnl. I just added two new commits, one fixing the sniff to handle custom nonce verification functions correctly regardless of the case, and another adding tests safeguarding that non-ASCII characters are handled properly. |
| $this->customNonceVerificationFunctions = array_map( 'strtolower', $this->customNonceVerificationFunctions ); | ||
|
|
There was a problem hiding this comment.
Why are you changing the user-provided value ? instead of changing the merge result $this->nonceVerificationFunctions ?
There was a problem hiding this comment.
I had in my mind that the user-provided value is the only one that required change, and that is why I opted to change it. After seeing your question, I imagine you are suggesting changing $this->nonceVerificationFunctions to ensure the sniff behaves correctly if, in the future, a new function is added to this property without all lowercase letters, and also to ensure it behaves correctly for sniffs extending this one. Is that the case?
I went ahead and pushed a new commit changing $this->nonceVerificationFunctions instead of $this->customNonceVerificationFunctions.
The sniff was incorrectly treating function names as case-sensitive when checking for nonce verification functions. This led to false positives when developers used valid nonce verification functions with mixed or uppercase letters.
The fix ensures function names are properly converted to lowercase before comparing against the allowed nonce verification functions list, respecting PHP's case-insensitive function name behavior.